x/distribution/CHANGELOG.md (1)

Line range hint 1-1: Use standard Markdown strikethrough syntax.

For reverted changes, consider using the standard Markdown strikethrough syntax ~~text~~ instead of ~text~. This ensures better compatibility across different Markdown viewers.

Example:

-~The distribution module keeper now takes a new argument `PoolKeeper` in addition.~ Reverted on #20790
+~~The distribution module keeper now takes a new argument `PoolKeeper` in addition.~~ Reverted on #20790

x/distribution/keeper/genesis.go (1)

126-127: LGTM: Consistent balance check implementation

The implementation aligns well with the changes in the gov module. However, the error message could be more descriptive.

Consider enhancing the error message:

-		panic(fmt.Sprintf("distribution module balance is less than module holdings: %s < %s", balances, moduleHoldingsInt))
+		panic(fmt.Sprintf("distribution module balance insufficient - required: %s, actual: %s", moduleHoldingsInt, balances))

Configuration used: .coderabbit.yml
Review profile: CHILL

#!/bin/bash
# Check for similar balance checks in other modules
rg -A 2 "balance.*IsAllGTE|balance.*IsAllLT" --type go

x/distribution/keeper/genesis.go (1)

126-127: LGTM! Balance check logic is improved.

The change correctly implements the requirement to ensure sufficient balance rather than exact balance. Consider making the error message more specific:

-		return fmt.Errorf("distribution module balance is less than module holdings: %s < %s", balances, moduleHoldingsInt)
+		return fmt.Errorf("%s module balance is less than module holdings: %s < %s", types.ModuleName, balances, moduleHoldingsInt)

Configuration used: .coderabbit.yml
Review profile: CHILL

tests/integration/distribution/genesis_test.go (2)

3-25: Improve import organization for better readability

The imports should be grouped in the following order:

1. Standard library
2. External dependencies
3. Internal dependencies

import (
	"encoding/json"
	"testing"

	abci "github.com/cometbft/cometbft/api/cometbft/abci/v1"
	"github.com/stretchr/testify/suite"

	corestore "cosmossdk.io/core/store"
	coretesting "cosmossdk.io/core/testing"
	"cosmossdk.io/depinject"
	"cosmossdk.io/log"
	sdkmath "cosmossdk.io/math"
	"cosmossdk.io/x/distribution/keeper"
	"cosmossdk.io/x/distribution/types"
	bankkeeper "cosmossdk.io/x/bank/keeper"
	stakingkeeper "cosmossdk.io/x/staking/keeper"

	"github.com/cosmos/cosmos-sdk/codec"
	"github.com/cosmos/cosmos-sdk/runtime"
	simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
	sdk "github.com/cosmos/cosmos-sdk/types"
	_ "github.com/cosmos/cosmos-sdk/x/auth"
	authkeeper "github.com/cosmos/cosmos-sdk/x/auth/keeper"
)

🧰 Tools

🪛 golangci-lint (1.62.2)

6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order

(gci)

---

11-11: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order

(gci)

---

15-15: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order

(gci)

---

20-20: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order

(gci)

---

175-188: Add documentation for the clearDB helper

Consider adding a comment explaining the purpose of this helper function and why it's necessary between tests.

+// clearDB removes all entries from the test database to ensure a clean state between tests.
 func (s *ImportExportSuite) clearDB(db corestore.KVStoreWithBatch) {

tests/integration/gov/genesis_test.go (1)

241-327: LGTM with suggestions for improvement

The new test case effectively validates the insufficient balance scenario. However, consider these improvements:

1. The error assertion could be more specific to ensure the exact error condition is being tested
2. Consider adding a test case for the boundary condition where the balance exactly equals the required amount

For the error assertion, consider:

-require.ErrorContains(t, err, "expected gov module to hold at least")
+expectedErr := fmt.Sprintf("expected gov module to hold at least %s, but only has %s", 
+    params.MinDeposit.String(),
+    sdk.Coins(params.MinDeposit).QuoInt(sdkmath.NewInt(2)).String())
+require.ErrorContains(t, err, expectedErr)

Configuration used: .coderabbit.yml
Review profile: CHILL

tests/integration/distribution/genesis_test.go (1)

67-125: LGTM! Comprehensive happy path test.

The test effectively verifies the new behavior allowing module balance to exceed total rewards. Consider adding test cases for edge cases where the balance is exactly equal to rewards.

// Add test case for exact balance match
rewards := sdk.NewDecCoinsFromCoins(sdk.NewCoin("stake", sdkmath.NewInt(33)))
err = s.BankKeeper.SendCoinsFromAccountToModule(ctx, s.addrs[0], types.ModuleName, sdk.NewCoins(sdk.NewCoin("stake", sdkmath.NewInt(33))))

Configuration used: .coderabbit.yml
Review profile: CHILL

tests/integration/v2/gov/genesis_test.go (1)

168-230: Good test coverage, consider enhancing error validation

The test effectively validates the insufficient balance scenario, which is crucial for the PR's objective. However, consider these enhancements:

1. Add assertions to verify the intermediate state after sending coins from the governance module (around line 192)
2. Validate the specific error message content more thoroughly beyond just the prefix

Example enhancement:

 err = s1.BankKeeper.SendCoinsFromModuleToAccount(ctx, types.ModuleName, addrs[0], sdk.Coins(params.MinDeposit).QuoInt(sdkmath.NewInt(2)))
 require.NoError(t, err)
+
+// Verify intermediate state
+macc := s1.GovKeeper.GetGovernanceAccount(ctx)
+balance := s1.BankKeeper.GetAllBalances(ctx, macc.GetAddress())
+require.True(t, balance.IsAllLT(sdk.Coins(params.MinDeposit)), "governance account balance should be less than minimum deposit")
+
 authGenState, err := s1.AuthKeeper.ExportGenesis(ctx)

tests/integration/distribution/genesis_test.go (5)

27-38: Add documentation for the test suite

Consider adding a doc comment explaining the purpose of this test suite and its role in validating genesis import/export functionality, particularly around balance assertions.

---

46-46: Document the significance of the token amount

The value 42 appears to be a magic number. Consider either documenting why this specific value was chosen or using a named constant.

-valTokens := sdk.TokensFromConsensusPower(42, sdk.DefaultPowerReduction)
+// Using 42 as a reasonable amount that ensures sufficient voting power for testing
+const testValidatorTokens = 42
+valTokens := sdk.TokensFromConsensusPower(testValidatorTokens, sdk.DefaultPowerReduction)

---

67-95: Enhance test assertions for module balance

The test verifies validator rewards but doesn't assert the final module balance. Consider adding balance verification to ensure complete test coverage.

+// Verify final module balance
+moduleAddr := s.AccountKeeper.GetModuleAddress(types.ModuleName)
+moduleBalance := s.BankKeeper.GetAllBalances(ctx, moduleAddr)
+s.Require().Equal(
+    sdk.NewCoins(sdk.NewCoin("stake", sdkmath.NewInt(34))),
+    moduleBalance,
+    "unexpected module balance",
+)

---

127-139: Enhance test documentation and error handling

The test setup could be more explicitly documented to clarify the expected failure scenario. Consider adding a detailed comment explaining why 35 tokens of rewards with only 34 tokens in the module should fail.

+// TestInsufficientFunds verifies that genesis import fails when the distribution
+// module balance (34 tokens) is less than the total outstanding rewards (35 tokens).
+// This aligns with the requirement that the module must have sufficient funds to
+// cover all outstanding rewards.
 func (s *ImportExportSuite) TestInsufficientFunds() {

---

177-190: Document helper method and consider batch operations

1. Add documentation explaining the purpose and importance of clearing the database between tests.
2. Consider using batch operations for better performance when deleting multiple keys.

+// clearDB removes all entries from the test database to ensure a clean state
+// between test runs. This prevents any potential state leakage that could
+// affect test results.
 func (s *ImportExportSuite) clearDB(db corestore.KVStoreWithBatch) {
     iter, err := db.Iterator(nil, nil)
     s.Require().NoError(err)
     defer iter.Close()
 
     var keys [][]byte
     for ; iter.Valid(); iter.Next() {
         keys = append(keys, iter.Key())
     }
 
+    batch := db.NewBatch()
     for _, k := range keys {
-        s.Require().NoError(db.Delete(k))
+        s.Require().NoError(batch.Delete(k))
     }
+    s.Require().NoError(batch.Write())
 }

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro